More mdcheck fixes: Rework mdcheck service logic (2nd attempt) #198
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The way mdcheck is currently written, it loops over /dev/md?*, which will contain RAID partitions and arrays that don't support sync operations, such as RAID0. This is inefficient and makes the script difficult to trace. Instead, loop over the sync_action files which actually matter for checking. Signed-off-by: Martin Wilck <[email protected]>
This syntax used to be marked as deprecated [1]. In current bash man pages, it isn't even mentioned any more. Use the POSIX compatible syntax "$((X+=1))" instead [2, 3]. [1] https://stackoverflow.com/questions/41081417/difference-between-a-b-and-a-b-in-bash [2] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_04 [3] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap01.html#tag_17_01_02_01 Signed-off-by: Martin Wilck <[email protected]>
If the script is run from systemd, simply writing to stderr will create more concise output. Otherwise, use logger like before. Use the same logic for other messages printed by the script during runtime, except for error messages related to the invocation. Signed-off-by: Martin Wilck <[email protected]>
The current logic of "mdcheck" is susceptible to races when multiple mdcheck instances run simultaneously, as checks can be initiated from both "mdcheck_start.service" and "mdcheck_continue.service". The previous commit 8aa4ea9 ("systemd: start mdcheck_continue.timer before mdcheck_start.timer") fixed this for the default configuration by inverting the ordering of timers. But users can customize the timer settings, which can cause the race described in the commit message of 8aa4ea9 to reappear. This patch avoids this kind of race entirely, by changing the logic as follows: * When `mdcheck` has finished checking a RAID array, it will create a marker `/var/lib/mdcheckChecked_$UUID`. * A new option `--restart` is introduced. `mdcheck --restart` removes all `/var/lib/mdcheck/Checked_*` markers. This is called from `mdcheck_start.service`, which is typically started by a timer in large time intervals (default once per month). * `mdcheck --continue` works as it used to. It continues previously started checks (where the `/var/lib/mdcheck/MD_UUID_$UUID` file is present and contains a start position) for the check. This usage is *not recommended any more*. * `mdcheck` with no arguments is like `--continue`, but it also starts new checks for all arrays for which no check has previously been started, *except* for arrays for which a marker `/var/lib/mdcheck/Checked_$UUID` exists. `mdcheck_continue.service` calls `mdcheck` this way. It is called in short time intervals, by default once per day. * Combining `--restart` and `--continue` is an error. This way, the only systemd service that actually triggers a kernel-level RAID check is `mdcheck_continue.service`, which avoids races. When all checks have finished, `mdcheck_continue.service` is no-op. When `mdcheck_start.service` runs, the checks re re-enabled and will be started from 0 by the next `mdcheck_continue.service` invocation. Signed-off-by: Martin Wilck <[email protected]>
The previous patch changed the behavior of mdcheck when invoked without --continue or --restart. Previously it would have started a new check from zero on all arrays. After the previous patch, it acted like --continue on arrays where a check had been started already, and started a new check only for arrays where no Checked_* marker was present. Introduce a new run mode --maybe-start for this behavior, and another mode --force-start for the original behavior. --maybe-start is the mode which will be called by mdcheck_continue.service now. For backward compatibilty reasons, --force-start is the default when no mode argument (--continue, --maybe-start, --restart, or --force-start) is specified. Because this is may interrupt checks that have already made progress, the tool will print a warning and pause for a short time when it's invoked in this manner. Signed-off-by: Martin Wilck <[email protected]>
mtkaczyk
reviewed
Oct 14, 2025
| # If the script is run from systemd, simply write to the journal on stderr. | ||
| # Otherwise, use logger. | ||
| log() { | ||
| if [[ "$INVOCATION_ID" ]]; then |
There was a problem hiding this comment.
Could you please explain why INVOCATION_ID means that it was run from systemd?
mtkaczyk
reviewed
Oct 14, 2025
| # Modes are: | ||
| # --continue Don't start new checks, only continue previously started ones | ||
| # for which MD_UUID_$UUID already exists | ||
| # --maybe-start Like --continue, but also start new checks for arrays for which |
There was a problem hiding this comment.
can't we have --maybe-start replaced by: -continue --start-unchecked-arrays?
"maybe" seems weird as cmdline option.
There was a problem hiding this comment.
To keep backward compatibility, we don't need the --force-start option, right? The logic of the last patch can make sure all arrays will start check from 0 as default. So we don't need to add a new option --force-start. This can be simpler.
So we only need to add a new option for mdcheck_continue.service which can start/continue. How about kickoff? -continue --start-unchecked-arrays is also a little complicated.
Thanks
Xiao
|
Overall looks fine to me but I'm not an expert of |
XiaoNi87
reviewed
Oct 15, 2025
| eval MD_${i}_fl= | ||
| rm -f $fl | ||
| rm -f "$fl" | ||
| touch "${fl/MD_UUID_/Checked_}" |
There was a problem hiding this comment.
Hi Martin
If mdcheck is interrupted when sleep 220, there is a possibility that it misses creating Checked file. In cleanup function, it only removes $fl if it finishes check, but not creating Checked file.
|
Hi Martin The checkpatch reports some errors about format against the last patch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR on top of #189. Relaunch of #190 after @XiaoNi87 indicated to me that my changes would be acceptable to him.
This PR changes the logic of the "mdcheck" tool and the related systemd services
mdcheck_start.serviceandmdcheck_continue.service.The current behavior is like this:
mdcheckwithout arguments starts a RAID check on all arrays on the system, starting at position 0. This is started frommdcheck_start.service, started by a systemd timer once a month.mdcheck --continuelooks for files/var/lib/mdcheck/MD_UUID_$UUID, reads the start position from them, and starts a check from that position on the array with the respective UUID. This is started from a systemd timer every night.In either case,
mdcheckwon't do anything if the kernel is already running async_actionon a given array. The check runs for a given period of time (default 6h) and saves the last position in theMD_UUIDfile, to be taken up whenmdcheck --continueis called next time. When the entire array has been checked, theMD_UUID_file is deleted. When all checks are finished,mdcheck_continue.timeris stopped, to be restarted whenmdcheck_start.timerexpires next time.Before the recent commit 8aa4ea9 ("systemd: start mdcheck_continue.timer before mdcheck_start.timer"), this could lead to a race condition when the check for a given array didn't complete throughout the month.
mdcheck_start.servicewould start and reset the check position to 0 beforemdcheck_continue.servicecould pick up at the last saved position. 8aa4ea9 works around this by starting mdcheck_continue.service a few minutes before mdcheck_start.timer.Yet the general problem still exists: both services trigger checks on the kernel's part which they can only passively monitor. So if a user plays with the timer settings (which he is in his rights to do), another similar race might happen.
This patch set changes the behavior as follows:
Only
mdcheck_continue.serviceactually starts and stops kernel-based sync actions. This service will continue at the saved start position if anMD_UUID*file exists, or start a new check at position 0 otherwise. Starting at 0 can be inhibited by creating a file/var/lib/mdcheck/Checked_$UUID. These files will be created bymdcheckwhen it finishes checking a given array. Thus future invocations ofmdcheck_continue.servicewill not restart the check on this array.mdcheck_start.servicerunsmdcheck --restart, which simply removes allChecked_*markers from/var/lib/mdcheck, so that the next invocation ofmdcheck_continue.servicewill start new checks on all arrays which don't have already running checks.The general behavior of the systemd timers and services is like before, but the mentioned race condition is avoided, even if the user modifies the timer settings arbitrarily.
Unlike #190, this PR preserves the behavior of the
mdcheckscript when called without arguments.mdcheckhistorically had just two modes of operation: default (no arguments) and--continue. This set introduces new modes--restart(was in #190 already),--maybe-start(the behavior ofmdcheckwithout args in #190), and--force-start(a complete restart of checks on all arrays from 0, like traditionalmdcheck). For backward compatibility reasons,--force-startbecomes the default behavior.More details in the commit descriptions.
Differences to #190:
mdcheck